-
-
Notifications
You must be signed in to change notification settings - Fork 614
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add options for minimal links and auto-prefixing http protocol #727
Conversation
src/trumbowyg.js
Outdated
const VALID_LINK_PREFIX = /^([a-z][-+.a-z0-9]*:|\/|#)/i; | ||
if(VALID_LINK_PREFIX.test(url)) { return url; } | ||
|
||
return url.includes("@") ? `mailto:${url}` : `http://${url}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's wrong, you can use @ in HTTP url to put some user/password:
[//[user[:password]@]host[:port]][/path][?query][#fragment]
Thanks for your Pull Request :) Yep, do not use ES6 since I did not use babel and want to be ES5-compatible. |
Cheers for the feedback - I'll update the ES6 bits and implement a regex to distinguish email addresses / urls. Be back with you soon 👍 |
Simple regex used for testing whether link input is an email rather than a url.
I suggest to rename it to autoPrefixProtocol and have an urlProtocol option which defaults to https, but can be set to e.g. Http. Https will be the new Standart since chrome will also mark websites as untrusted soon when they use http. |
Thanks, sounds sensible. I've updated the option name. Under the new setup, users can provide the following options to
Anything else will be ignored. Let me know what you think - appreciate your work on this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please stick to the eslint configuration to keep coherence :)
src/trumbowyg.js
Outdated
|
||
if(typeof(protocol) === 'boolean') { | ||
return 'https://' | ||
} else if(typeof(protocol) === 'string') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need else as we always manage all others cases with the previous if
src/trumbowyg.js
Outdated
|
||
if(!protocol) { return; } | ||
|
||
if(typeof(protocol) === 'boolean') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (typeof protocol !== 'string') {
src/trumbowyg.js
Outdated
target: { | ||
label: t.lang.target, | ||
value: target | ||
value: new XMLSerializer().serializeToString(documentSelection.getRangeAt(0).cloneContents()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one is out of date and should stay text
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from my side this looks fine now
src/trumbowyg.js
Outdated
} | ||
if(typeof(protocol) !== 'string') { return 'https://'; } | ||
if(/^https?:\/\/$/.test(protocol)) { return protocol; } | ||
if(/^https?$/.test(protocol)) { return protocol + '://'; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a lot of code to just get https://
or http://
:/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about others protocols? If I want put some ftp:// or whatever?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Alex-D.
Would you prefer to allow any string value in the option, and append ://
if it's missing?
So:
- a
true
value defaults tohttps://
- a string without the correct suffix has
://
appended to it - a string with the correct suffix is prepended as is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep I think it could be cool :)
Morning @Alex-D - that's the
How does that look to you? |
LGTM :) Could you update the doc about this new option? It's somewhere here:
Thanks! |
Fantastic - thanks @Alex-D. Really appreciate all your work with me on this. Let me know what you think of the updated docs and I'll edit if needed. One thing to note: I've put |
@Alex-D - please don't consider this a chase, but I'm wondering what your process is for introducing new features such as these, and when you're next likely to do so? Hope I'm not being a pain asking this :) |
Just taking time to run this on my computer, then merge other MRs, release a new version, test it, etc. So I need to take couple of hours ^^' Sorry if it's long... |
Thanks, that clears things up perfectly. It was more a case of establishing whether this would be deployed shortly or as part of a larger release way down the line. |
Thanks! :) |
Was inserted in v2.10.0 / Alex-D#727.
This PR (and release 2.10.0) broke ES5 browsers/minifiers because of the use of See #764. |
Two options added:
autoPrefixHttpProtocol
andminimalLinks
.minimalLinks
slims down the link overlay to use onlytext
andurl
.autoPrefixHttpProtocol
adjust links without a protocol. Those with a protocol, anchors and relative links are left alone. Anything else if prepended withhttp://
ormailto:
as appropriate.Both new options default to
false
.N.B. ES6 methods used for string interpolation. Can adjust if required.